Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url: drop auth in url.resolve() if host changes #1480

Closed
wants to merge 1 commit into from

Conversation

rlidwka
Copy link
Contributor

@rlidwka rlidwka commented Apr 20, 2015

#1435

Not sure how to handle this though.

@Fishrock123 Fishrock123 added the url Issues and PRs related to the legacy built-in url module. label Apr 20, 2015
@brendanashworth
Copy link
Contributor

I wonder what should happen in this case?

url.resolve('mailto:user@example.org', 'example.com')
'mailto:user@example.com'

"user@" technically is auth info here.

I don't think it should copy over. However, to change that would be semver-major imo. This would be better:

> var parsed = url.parse('mailto:user@example.org');
undefined
> parsed.host = 'example.com';
'example.com'
> url.format(parsed);
'mailto:user@example.com'

@brendanashworth
Copy link
Contributor

perhaps @domenic would be interested in reviewing?

@domenic
Copy link
Contributor

domenic commented Jul 21, 2015

What do browsers do? What does the spec (perhaps best tested via https://github.com/jsdom/whatwg-url) do?

@trevnorris
Copy link
Contributor

I agree with @domenic. Our url module should align with the spec.

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

Looks like this was never resolved. There's really no question that the user id and password should not be getting copied over.. url.resolve('http://user:pass@example.org', 'http://example.com') should never resolve out to http://user:pass@example.com. AFAICT, that aligns with the url spec also.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@nodejs/http

@dougwilson
Copy link
Member

I concur with @jasnell and this PR

@jasnell
Copy link
Member

jasnell commented Apr 22, 2016

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 22, 2016
@jasnell
Copy link
Member

jasnell commented Apr 22, 2016

@nodejs/ctc ... amazingly, this PR was opened a year ago and still applies cleanly (albeit using a three way merge). It even passes linting! The change LGTM.

marked it semver-major because it changes the behavior of url.resolve to drop the auth but it could also be classified as a bug fix. PTAL

@jasnell jasnell added this to the 6.0.0 milestone Apr 22, 2016
@jasnell
Copy link
Member

jasnell commented Apr 23, 2016

CI is green!

@jasnell
Copy link
Member

jasnell commented Apr 25, 2016

@mscdex @cjihrig @trevnorris ... can one of you give this a quick glance over?

'http://diff:auth@www.example.com/']
'http://diff:auth@www.example.com/'],

// https://github.com/iojs/io.js/issues/1435
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be changed to point to the nodejs/node repo

Copy link
Member

@jasnell jasnell Apr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I was going to change that upon landing (although, I kinda like that it still points to iojs, lol)

@mscdex
Copy link
Contributor

mscdex commented Apr 25, 2016

LGTM

jasnell pushed a commit that referenced this pull request Apr 25, 2016
Fixes: #1435
PR-URL: #1480
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Apr 25, 2016

It only took 1 year and 5 days but this landed in eb4201f ;-)

@jasnell jasnell closed this Apr 25, 2016
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Fixes: #1435
PR-URL: #1480
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants